-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Decode single frame using GDCM #422
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great follow-up on the new frame decoding feature, much appreciated!
I assume that this is still a work in progress, but I will leave some comments for now.
.for_each(|fragment| buf.append(&mut fragment.clone())); | ||
buf | ||
} else { | ||
fragments[frame].to_vec() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the frame comprises more than one fragment in a multi-frame pixel data fragment sequence? I do not think that we can make this assumption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only single frame dicoms can have more than 1 fragment. Multi frame files, must be 1 fragment per frame. At least according to the standard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can move forward with this assumption and revisit if we find a conflicting sample case. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a heads-up, I received clarifications from DICOM WG6, who asserted that it is allowed for frames in a multi-frame instance to span through multiple fragments each, though more recent transfer syntaxes tend to disallow this.
Still, I won't make this a blocker so that these improvements can be delivered. The affected area is only for the integration with GDCM, which hopefully the project will depend less on over time.
So far, I have tested this with Compressed (jpeg, jpeg-ls and j2k) and Uncompressed (little endian) data. With RGB, YBR_FULL_422, MONOCHROME1, MONOCRHOME2 and PALETTE_COLOR (at least decompressing) photometric interpretations. |
Hello @dougyau! Are there any other pending concerns with this PR, so that it can be marked as ready? |
This code is working, what I have to do is cover it with unit testing. I have been short of time, but once I do have some time, I will add the tests. |
I have validated the existing test, and it uses the new code path. Of the test cases I believe we could convert |
I have been slowly working on a new release for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this with a few test files and could validate that the behavior has improved in accuracy and performance after these changes. I believe we are better off merging this now and reiterating on the test suite after dicom-test-files
is updated. Thanks!
Extending from the PR #421 when using GDCM to decode images it will load the whole image to memory. This PR implements the logic required to load only one image.